-
Notifications
You must be signed in to change notification settings - Fork 5.2k
Mark HttpClient.Send tests uniformly with UnsupportedOSPlatform for Android, iOS #120716
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Mark HttpClient.Send tests uniformly with UnsupportedOSPlatform for Android, iOS #120716
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses issue #104716 by adding platform-specific annotations to mark HttpClient.Send methods as unsupported on Android and iOS platforms, in addition to the existing browser platform restriction.
Key changes:
- Added
UnsupportedOSPlatformattributes for "android" and "ios" to synchronous Send methods - Uncommented previously commented-out platform attributes in HttpClientHandler
- Updated both implementation and reference assembly files for consistency
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| HttpMessageInvoker.cs | Added android and ios platform restrictions to the base Send method |
| HttpClientHandler.AnyMobile.cs | Uncommented android and ios platform attributes for the Send method override |
| HttpClient.cs | Added android and ios platform restrictions to all four Send method overloads |
| System.Net.Http.cs | Updated reference assembly to reflect the new platform restrictions |
Comments suppressed due to low confidence (1)
src/libraries/System.Net.Http/ref/System.Net.Http.cs:1
- Corrected malformed attribute name from 'UnsupportedOSPlatformAttributeUnsupportedOSPlatform' to 'UnsupportedOSPlatform'.
// Licensed to the .NET Foundation under one or more agreements.
|
Tagging subscribers to this area: @dotnet/ncl |
| [System.Runtime.Versioning.UnsupportedOSPlatformAttribute("browser")] | ||
| [System.Runtime.Versioning.UnsupportedOSPlatform("android")] | ||
| [System.Runtime.Versioning.UnsupportedOSPlatform("browser")] | ||
| [System.Runtime.Versioning.UnsupportedOSPlatform("ios")] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why it doens't also exclude "tvos"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ManickaP
Ok, good catch., thanks!
I will add "tvos" on the unsupportedOSPlatform today.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ManickaP
Sorry for long reply. Done updating to add tvos.
Please review.
…d, ios over eixisting
accept copilot suggestion to correct UnsupportedOSPlatform attribute name Co-authored-by: Copilot <[email protected]>
f910d01 to
5066cb7
Compare
|
Rebase again to |
5066cb7 to
0ebf851
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
|
Seems like the errors are relevant: |
@ManickaP especially this comment: dotnet/arcade#7585 (comment) |
|
No, you have to add the same set of attributes to |
Fixes #104716 by marking HttpClient.Send with UnsupportedOSPlatform with values of android, ios over eixisting.
Please review.
cc @wfurt , @ManickaP